Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow _.throttle to function correctly after the system time is updated #1473

Merged

Conversation

blake-simpson
Copy link
Contributor

I have found that if the system time is moved backwards, calls to throttle will stop executing.

To test this, if the system time is moved backwards by an hour then the remaining variable in _.throttle containing the time offset becomes a large positive integer and the throttle callback will not be called until the adjusted hour time difference has passed, instead of the expected wait time.

For this reason I suggest that an additional check is made to see if remaining exceeds the wait time which will then clear the existing timeout and return the method to a normal state.

I have tested this in a project and provided a unit test as proof.

@akre54
Copy link
Collaborator

akre54 commented Feb 13, 2014

This is mainly useful for things like Daylight Savings Time or going between time zones, right? Otherwise this seems like an incredibly slight edge case.

@melnikov-s
Copy link
Contributor

Seems like _.debounce has the same problem. Small nitpick is that if a system clock is moved back , _.throttle will execute on the next hit, regardless if wait has passed or not.

Perhaps it's worth considering removing Date all together and coming up with a pure setTimeout solution for both _.debounce and _.throttle. The timer won't be adjusted for 100% accuracy but at least we won't have these edge cases.

@blake-simpson
Copy link
Contributor Author

@akre54 This is one case, the project where I use this is a browser game based in many countries. Apparently players will adjust the system clock to match their local time difference to the servers.

@smelnikov Unfortunately yes, when the system time changes this fix may trigger a callback prematurely but I believe this is not a huge issue, as most wait offsets are small and after the initial callback, the timer will be cleared and state will return to normal.

This fix could also be applied to _.debounce too, if the solution is found to be acceptable.

@akre54
Copy link
Collaborator

akre54 commented Feb 13, 2014

This is probably too edge-case for Underscore. You might mixin your own timezone-independent throttle for your specific needs...

@jashkenas
Copy link
Owner

This is probably too edge-case for Underscore

Nah, this is a thing that can happen. Especially for long-lived single page apps that you don't close. Let's add the sanity check fix for this and for debounce.

@michaelficarra
Copy link
Collaborator

This additional test isn't sufficient for protection from leap seconds or when the delay time is long. Consider the case of a 24 hour delay. If it goes past a DST boundary, it may take either 23 or 25 hours, and this PR won't necessarily account for that.

@jashkenas
Copy link
Owner

When _.throttle has a delay time of 24 hours? Now that's an edge case, in the browser ;)

@michaelficarra
Copy link
Collaborator

You know what I mean. Any time over an hour, or in the case of a leap second, any time over a second.

@blake-simpson
Copy link
Contributor Author

I believe the this won't be an issue, after reading ECMAScript Spec §15.9.1.1 it appears timestamps from Date.getTime() are normalised over time zones and leap seconds are ignored.

Taking this into account, I have also added a sanity check to _.debounce + a unit test for this.

@grahamscott
Copy link

It would appear that this fix also fixes issues with Sinon.js, specifically tests on debounced functions that use sinon.useFakeTimers (https://github.com/cjohansen/Sinon.JS/blob/master/lib/sinon/util/fake_timers.js)

@grahamscott
Copy link

I spoke to soon. It seems that there are random failures with useFakeTimers on debounced functions with underscore 1.6

@lfac-pt
Copy link

lfac-pt commented Feb 15, 2014

@grahamscott you can find more info on the fake timers issue at #1478, if you haven't seen it already.

@grahamscott
Copy link

@grahamscott you can find more info on the fake timers issue at #1478, if you haven't seen it already.

Thank you!

jashkenas added a commit that referenced this pull request Mar 3, 2014
Allow _.throttle to function correctly after the system time is updated
@jashkenas jashkenas merged commit 7479a24 into jashkenas:master Mar 3, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants